@MichaelPetrinolis what do you think?

@sebastienros when someone registers an AAD application for accounts in any organizational directory, he allows anyone with an AAD account to authenticate through this app.

@gvkries I think that we should also define the audiences (tenants) we wish to give access to the OC site and validate for those audiences, otherwise anyone with an enterprise Microsoft account could authenticate and then the script could potentially assign app roles to him.

If needed, an option to allow any tenant should be explicitly selected by the admin when configuring Microsoft Entra ID, but we should clearly state the consequences and provide an example with a secure script to assign roles (check the issuer of the claims etc.)

//cc: @kevinchalet what are your thoughts on this?

This PR just fixes the missing validation when multiple Azure AD tenants should be allowed. It must be explicitly configured in the Orchard configuration by setting the tenant ID to "common", so it has no security implications by default.

Configuring the allowed tenants would be a nice addition, but in many cases this is not the intention. We want to allow any user to register with their organization account the same way as with personal Microsoft (or other) accounts. It is also already possible to use the registration script to configure allowed tenants, by inspecting the tenant ID claim.

@gvkries it is nice that to enable it apart from configuring the App Registration in Entra ID you must also explicitly configure OC (we should update the docs about setting the value 'common' to TenantId). My concern is that we should also make it clear to anyone who enables the feature to correctly configure any role assignments through script or code with additional checks to avoid any security side effects. Unfortunately, it is not possible for the moment to deny login with the IExternalLoginEventHandler interface.
Indeed, configuring a valid audience (list of allowed tenants) could be another PR unless Entra ID already supports it.

@MichaelPetrinolis that is true, one cannot deny login with the script. I'm not that concerned about the login script, because it also must be explicitly written by an admin. There are no defaults that assign any roles at all, even the app registration must be configured manually before. Anyone configuring the authentication/registration settings are already exposed to a lot of possibly dangerous settings and hopefully know what they are doing. But documentation is always a good thing 😊

Please don't forget to re-request review from the top-right corner, otherwise reviewers won't know that you're done.

Great, thank you!